-
Notifications
You must be signed in to change notification settings - Fork 4
Tabs feature #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Tabs feature #27
Conversation
…s to gitignore for development.
…or this (tabs, history, activeTab). Added chrome.png. Added the connection to connection_manager. Added background script in background.ts to get all tabs in the browser. This connection also has 2 error handlers (not enough tabs and no tabs found) that more-readably show to the user when either is the case.
…b contents for better space creation. Added scripting permission to package.json to allow for tab reading to occur.
Summary of ChangesHello @Vodiboi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a major new capability to integrate and analyze a user's open Chrome browser tabs. It enables the application to gather information from active web pages, process their content, and present it in a structured 'space'. The changes encompass significant additions to the background script for tab and content handling, updates to extension permissions, and a new UI component for user interaction, alongside general improvements to data processing and error resilience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new 'Tabs' feature, which creates a Mantis space based on the content of currently open browser tabs. The implementation includes a new background script logic to fetch tab content, a new connection ChromeTabsConnection, and UI integration on the Google search page. Additionally, it refactors UUID generation by removing the uuid package in favor of a centralized helper.
My review focuses on a few key areas. There's a critical issue in src/background.ts where multiple onMessage listeners are used, which should be consolidated into one to prevent unpredictable behavior. I've also pointed out some opportunities to improve code quality in the new chromeTabs connection by using specific types instead of any and adhering to best practices for event handling. Using console.error for logging errors is also recommended for better diagnostics.
| chrome.runtime.onMessage.addListener((request, sender, sendResponse) => { | ||
| // Handle getTabs request for Chrome Tabs connection | ||
| if (request.action === "getTabs") { | ||
| chrome.tabs.query({}, (tabs) => { | ||
| if (chrome.runtime.lastError) { | ||
| sendResponse({ error: chrome.runtime.lastError.message }); | ||
| } else { | ||
| sendResponse({ tabs: tabs }); | ||
| } | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| // Handle getTabsWithContent request | ||
| if (request.action === "getTabsWithContent") { | ||
| chrome.tabs.query({}, async (tabs) => { | ||
| if (chrome.runtime.lastError) { | ||
| sendResponse({ error: chrome.runtime.lastError.message }); | ||
| return; | ||
| } | ||
|
|
||
| const tabsWithContent = []; | ||
|
|
||
| for (const tab of tabs) { | ||
| const tabData = { ...tab, pageContent: '' }; // Add pageContent property | ||
|
|
||
| // Try to get page content for each tab | ||
| try { | ||
| if (tab.id && tab.url && !tab.url.startsWith('chrome://') && !tab.url.startsWith('chrome-extension://')) { | ||
| // Execute content script to get page text | ||
| const results = await chrome.scripting.executeScript({ | ||
| target: { tabId: tab.id }, | ||
| func: getPageContent, // Use 'func' instead of 'function' | ||
| }); | ||
|
|
||
| if (results && results[0] && results[0].result) { | ||
| tabData.pageContent = results[0].result; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.log(`Could not get content for tab ${tab.id}:`, error); | ||
| // Set a fallback description | ||
| tabData.pageContent = `Content from ${tab.url ? new URL(tab.url).hostname : 'unknown site'} - unable to read page content`; | ||
| } | ||
|
|
||
| tabsWithContent.push(tabData); | ||
| } | ||
|
|
||
| sendResponse({ tabs: tabsWithContent }); | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| // Don't interfere with other message handlers | ||
| return false; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've added a new chrome.runtime.onMessage.addListener. However, this file now contains multiple onMessage listeners (e.g., another one starts on line 169, and more follow). Registering multiple listeners for the onMessage event is an anti-pattern in Chrome extension development and can lead to unpredictable behavior, race conditions, or messages not being handled as expected. Only one listener should be active for the runtime.onMessage event.
It's highly recommended to consolidate all message handling logic into a single chrome.runtime.onMessage.addListener block, using if/else if or a switch statement to delegate based on request.action.
Example of consolidation:
chrome.runtime.onMessage.addListener((request, sender, sendResponse) => {
switch (request.action) {
case "getTabs":
// ... logic for getTabs
return true; // Indicate async response
case "getTabsWithContent":
// ... logic for getTabsWithContent
return true; // Indicate async response
case "setCookie":
// ... logic for setCookie
return true; // Indicate async response
// ... other cases
default:
// Optional: handle unknown actions
break;
}
return false;
});There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something that should be fixed Luca? This problem predates my tabs feature, so I assume you did this intentionally...
| <button onclick="this.parentElement.remove()" style=" | ||
| background: rgba(255, 255, 255, 0.2); | ||
| border: 1px solid rgba(255, 255, 255, 0.3); | ||
| color: white; | ||
| padding: 8px 16px; | ||
| border-radius: 6px; | ||
| cursor: pointer; | ||
| ">Got it</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an inline onclick attribute is generally discouraged. It's better practice to attach event listeners programmatically using addEventListener. This improves separation of concerns (HTML structure vs. JS behavior) and makes the code more maintainable. A possible implementation would be to add an ID to the button, append the errorDiv to the body, then find the button by its ID and add the click listener. This also applies to the showNoTabsError function.
| <button onclick="this.parentElement.remove()" style=" | ||
| background: rgba(255, 255, 255, 0.2); | ||
| border: 1px solid rgba(255, 255, 255, 0.3); | ||
| color: white; | ||
| padding: 8px 16px; | ||
| border-radius: 6px; | ||
| cursor: pointer; | ||
| ">OK</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
type fix Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Hello @Vodiboi! I'm here to help. Do you have any questions about the summary I provided, or anything else regarding this pull request? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new 'Tabs' feature that creates a Mantis space from the content of all open browser tabs. The implementation includes adding a new Chrome Tabs connection, background script logic to fetch tab content, and the necessary permissions. The pull request also includes a good refactoring to replace the uuid package with the native crypto.randomUUID().
My review focuses on a critical issue in the new background script message listener that could break existing functionality. I've also provided suggestions to improve error messaging for a better user experience and to enhance type safety in the new connection for improved code quality and maintainability.
| // Don't interfere with other message handlers | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This return false is incorrect and can break other message listeners. In a Chrome extension's onMessage listener, you should only return true if you are sending a response asynchronously. If a message is not handled by this listener, it should have no explicit return (which defaults to undefined) to allow other registered onMessage listeners to process the message. Returning false can prematurely close the message port for other listeners that might need to send an asynchronous response, which could break existing functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new 'Tabs' feature, allowing users to create a 'space' from their open browser tabs. The implementation includes fetching tab data, extracting content via a content script, and adding a new connection. The PR also refactors UUID generation, which is a good improvement.
My review focuses on several key areas:
- Architecture: I've pointed out an issue with using multiple
onMessagelisteners inbackground.ts, which should be consolidated for robustness. - Maintainability & Best Practices: In the new
chromeTabsconnection, I've suggested improvements regarding magic numbers, brittle CSS selectors, and string-based error handling. - Security & Code Quality: I've highlighted the use of
innerHTMLwith inline event handlers, which is insecure, and suggested safer alternatives. I also noted areas where code can be simplified and type safety can be improved.
Overall, the feature is well-implemented, and these suggestions aim to improve the long-term quality and stability of the code.
| chrome.runtime.onMessage.addListener((request, sender, sendResponse) => { | ||
| // Handle getTabs request for Chrome Tabs connection | ||
| if (request.action === "getTabs") { | ||
| chrome.tabs.query({}, (tabs) => { | ||
| if (chrome.runtime.lastError) { | ||
| sendResponse({ error: chrome.runtime.lastError.message }); | ||
| } else { | ||
| sendResponse({ tabs: tabs }); | ||
| } | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| // Handle getTabsWithContent request | ||
| if (request.action === "getTabsWithContent") { | ||
| chrome.tabs.query({}, async (tabs) => { | ||
| if (chrome.runtime.lastError) { | ||
| sendResponse({ error: chrome.runtime.lastError.message }); | ||
| return; | ||
| } | ||
|
|
||
| const tabsWithContent = []; | ||
|
|
||
| for (const tab of tabs) { | ||
| const tabData = { ...tab, pageContent: '' }; // Add pageContent property | ||
|
|
||
| // Try to get page content for each tab | ||
| try { | ||
| if (tab.id && tab.url && !tab.url.startsWith('chrome://') && !tab.url.startsWith('chrome-extension://')) { | ||
| // Execute content script to get page text | ||
| const results = await chrome.scripting.executeScript({ | ||
| target: { tabId: tab.id }, | ||
| func: getPageContent, // Use 'func' instead of 'function' | ||
| }); | ||
|
|
||
| if (results && results[0] && results[0].result) { | ||
| tabData.pageContent = results[0].result; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error(`Could not get content for tab ${tab.id}:`, error); | ||
| // Set a fallback description | ||
| tabData.pageContent = `Content from ${tab.url ? new URL(tab.url).hostname : 'unknown site'} - unable to read page content`; | ||
| } | ||
|
|
||
| tabsWithContent.push(tabData); | ||
| } | ||
|
|
||
| sendResponse({ tabs: tabsWithContent }); | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| // Don't interfere with other message handlers | ||
| return false; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file now contains multiple chrome.runtime.onMessage.addListener calls. While this can work, it's an error-prone pattern that is hard to maintain and can lead to unexpected behavior if a listener doesn't correctly return true/false. A more robust and common pattern is to use a single onMessage listener that acts as a dispatcher, routing messages to the correct handler based on request.action (e.g., using a switch statement). I recommend refactoring to consolidate all message handling into a single listener.
| allText = allText | ||
| .replace(/\s+/g, ' ') // Replace multiple whitespace with single space | ||
| .replace(/\n+/g, ' ') // Replace newlines with spaces | ||
| .replace(/\t+/g, ' ') // Replace tabs with spaces | ||
| .trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| }); | ||
|
|
||
| // Check if we have enough data | ||
| if (extractedData.length < 3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } catch (error) { | ||
| console.error('Error in Chrome Tabs connection:', error); | ||
|
|
||
| const errorMessage = error.message || error.toString(); | ||
| if (errorMessage.includes('Dataset too small') || | ||
| errorMessage.includes('minimum 100 rows are required')) { | ||
| showDatasetTooSmallError(extractedData.length); | ||
| return null; | ||
| } | ||
|
|
||
| if (errorMessage.includes('Not enough tabs') || errorMessage.includes('No tabs found')) { | ||
| showNoTabsError(); | ||
| return null; | ||
| } | ||
|
|
||
| throw error; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handling errors by checking for substrings in the error message is brittle. If the error messages change, this logic will break. It's more robust to use custom error classes. For errors thrown within this file (like 'No tabs found' and 'Not enough tabs'), you can define and throw custom error classes and then check for them in the catch block using instanceof.
| } | ||
|
|
||
| // New function for automatic retry | ||
| const createSpaceWithAutoRetry = async (extractedData: any[], establishLogSocket: establishLogSocketType, title: string, maxRetries = 5) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extractedData parameter is typed as any[]. To improve type safety and code clarity, it's better to define a specific interface for the objects in this array. You could define an interface like this at the top of the file:
interface ExtractedTabData {
title: string;
semantic_title: string;
link: string;
snippet: string;
}Then you can use ExtractedTabData[] as the type for extractedData here and where it's initialized in createSpace.
| const showDatasetTooSmallError = (dataCount: number) => { | ||
| const errorDiv = document.createElement('div'); | ||
| errorDiv.style.cssText = ` | ||
| position: fixed; | ||
| top: 20px; | ||
| right: 20px; | ||
| background: linear-gradient(135deg, #ff6b6b, #ee5a52); | ||
| color: white; | ||
| padding: 20px; | ||
| border-radius: 12px; | ||
| box-shadow: 0 8px 32px rgba(0, 0, 0, 0.3); | ||
| z-index: 10000; | ||
| max-width: 400px; | ||
| font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, sans-serif; | ||
| `; | ||
|
|
||
| errorDiv.innerHTML = ` | ||
| <div style="display: flex; align-items: center; margin-bottom: 12px;"> | ||
| <strong style="font-size: 16px;">Not Enough Data</strong> | ||
| </div> | ||
| <p style="margin: 0 0 12px 0; line-height: 1.4; font-size: 14px;"> | ||
| We found ${dataCount} items, but need at least 100 to create a meaningful space. | ||
| </p> | ||
| <button onclick="this.parentElement.remove()" style=" | ||
| background: rgba(255, 255, 255, 0.2); | ||
| border: 1px solid rgba(255, 255, 255, 0.3); | ||
| color: white; | ||
| padding: 8px 16px; | ||
| border-radius: 6px; | ||
| cursor: pointer; | ||
| ">Got it</button> | ||
| `; | ||
|
|
||
| document.body.appendChild(errorDiv); | ||
| setTimeout(() => errorDiv.remove(), 8000); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function uses innerHTML and an inline onclick handler to create a UI element. This is not recommended due to potential security vulnerabilities (XSS) if the data were to come from an external source, and it makes the code harder to maintain. It's better to create DOM elements programmatically and attach event listeners using addEventListener. This advice also applies to the showNoTabsError function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handled, will commit in a few
| }; | ||
|
|
||
| const injectUI = async (space_id: string, onMessage: onMessageType, registerListeners: registerListenersType) => { | ||
| const menu = document.querySelector("#hdtb-sc > div > div > div.crJ18e")?.children[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query selector "#hdtb-sc > div > div > div.crJ18e" is very specific and relies on the current DOM structure and CSS classes of the Google search page. This is brittle and likely to break if Google updates their page design. If possible, try to find more stable selectors, like ones based on IDs, data-* attributes, or ARIA roles. If not, it's worth adding a comment to acknowledge this fragility. This also applies to other selectors in this file, like on line 264.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Now looks at the first 300 chars of text from a page and makes a space using it. Removed domain stuff and duplicating entries.